Replace `FxHashMap` with `IndexMap` in object properties #1547

raskad · 2021-09-01T01:20:06Z

This Pull Request fixes/closes #1539.

It changes the following:

Replace FxHashMap with IndexMap in object properties

Benchmarks look good for this. Minimal gains on most benches and some minimal losses.

raskad · 2021-09-01T02:04:25Z

Test262 conformance changes:

Test result	master count	PR count	difference
Total	80,685	80,685	0
Passed	32,784	32,838	+54
Ignored	15,818	15,818	0
Failed	32,083	32,029	-54
Panics	0	0	0
Conformance	40.63%	40.70%	+0.07%

Fixed tests (54):

test/built-ins/Object/keys/return-order.js [strict mode] (previously Failed)
test/built-ins/Object/keys/return-order.js (previously Failed)
test/built-ins/Object/keys/15.2.3.14-2-8.js [strict mode] (previously Failed)
test/built-ins/Object/keys/15.2.3.14-2-8.js (previously Failed)
test/built-ins/Object/keys/order-after-define-property.js [strict mode] (previously Failed)
test/built-ins/Object/keys/order-after-define-property.js (previously Failed)
test/built-ins/Object/keys/15.2.3.14-4-1.js [strict mode] (previously Failed)
test/built-ins/Object/keys/15.2.3.14-4-1.js (previously Failed)
test/built-ins/Object/create/15.2.3.5-4-40.js [strict mode] (previously Failed)
test/built-ins/Object/create/15.2.3.5-4-40.js (previously Failed)
test/built-ins/Object/defineProperties/15.2.3.7-6-a-93-1.js [strict mode] (previously Failed)
test/built-ins/Object/defineProperties/15.2.3.7-6-a-93-1.js (previously Failed)
test/built-ins/Object/defineProperties/15.2.3.7-6-a-93-3.js [strict mode] (previously Failed)
test/built-ins/Object/defineProperties/15.2.3.7-6-a-93-3.js (previously Failed)
test/built-ins/Object/assign/strings-and-symbol-order.js [strict mode] (previously Failed)
test/built-ins/Object/assign/strings-and-symbol-order.js (previously Failed)
test/built-ins/Object/entries/getter-adding-key.js [strict mode] (previously Failed)
test/built-ins/Object/entries/getter-adding-key.js (previously Failed)
test/built-ins/Object/entries/return-order.js [strict mode] (previously Failed)
test/built-ins/Object/entries/return-order.js (previously Failed)
test/built-ins/Object/entries/getter-making-future-key-nonenumerable.js [strict mode] (previously Failed)
test/built-ins/Object/entries/getter-making-future-key-nonenumerable.js (previously Failed)
test/built-ins/Object/entries/getter-removing-future-key.js [strict mode] (previously Failed)
test/built-ins/Object/entries/getter-removing-future-key.js (previously Failed)
test/built-ins/Object/entries/order-after-define-property.js [strict mode] (previously Failed)
test/built-ins/Object/entries/order-after-define-property.js (previously Failed)
test/built-ins/Object/entries/exception-during-enumeration.js [strict mode] (previously Failed)
test/built-ins/Object/entries/exception-during-enumeration.js (previously Failed)
test/built-ins/Object/values/getter-adding-key.js [strict mode] (previously Failed)
test/built-ins/Object/values/getter-adding-key.js (previously Failed)
test/built-ins/Object/values/return-order.js [strict mode] (previously Failed)
test/built-ins/Object/values/return-order.js (previously Failed)
test/built-ins/Object/values/getter-making-future-key-nonenumerable.js [strict mode] (previously Failed)
test/built-ins/Object/values/getter-making-future-key-nonenumerable.js (previously Failed)
test/built-ins/Object/values/getter-removing-future-key.js [strict mode] (previously Failed)
test/built-ins/Object/values/getter-removing-future-key.js (previously Failed)
test/built-ins/Object/values/exception-during-enumeration.js [strict mode] (previously Failed)
test/built-ins/Object/values/exception-during-enumeration.js (previously Failed)
test/built-ins/Reflect/ownKeys/return-array-with-own-keys-only.js [strict mode] (previously Failed)
test/built-ins/Reflect/ownKeys/return-array-with-own-keys-only.js (previously Failed)
test/built-ins/JSON/stringify/replacer-array-order.js [strict mode] (previously Failed)
test/built-ins/JSON/stringify/replacer-array-order.js (previously Failed)
test/built-ins/JSON/stringify/property-order.js [strict mode] (previously Failed)
test/built-ins/JSON/stringify/property-order.js (previously Failed)
test/built-ins/JSON/parse/reviver-object-get-prop-from-prototype.js [strict mode] (previously Failed)
test/built-ins/JSON/parse/reviver-object-get-prop-from-prototype.js (previously Failed)
test/language/statements/for-in/order-simple-object.js [strict mode] (previously Failed)
test/language/statements/for-in/order-simple-object.js (previously Failed)
test/language/statements/for-in/order-property-added.js [strict mode] (previously Failed)
test/language/statements/for-in/order-property-added.js (previously Failed)
test/language/statements/for-in/order-property-on-prototype.js [strict mode] (previously Failed)
test/language/statements/for-in/order-property-on-prototype.js (previously Failed)
test/language/statements/for-in/order-after-define-property.js [strict mode] (previously Failed)
test/language/statements/for-in/order-after-define-property.js (previously Failed)

jedel1043

Nice! Could you add a test to check if the order of insertion of object properties is preserved? Just to make sure we don't replace OrderedHashMap by accident with an unordered map in the future.

Razican · 2021-09-01T07:07:00Z

Benchmarks

┌─────────┬──────────────────────────────────────────────┬────────────────────┬─────────────────────┬────────────┐
│ (index) │                     name                     │  changesDuration   │   masterDuration    │ difference │
├─────────┼──────────────────────────────────────────────┼────────────────────┼─────────────────────┼────────────┤
│    0    │     'Arithmetic operations (Execution)'      │ '**227.5±1.15ns**' │   '229.2±0.31ns'    │  '-0.99'   │
│    1    │        'Arithmetic operations (Full)'        │ '**228.4±0.29µs**' │   '242.1±0.36µs'    │   '-5.7'   │
│    2    │          'Array access (Execution)'          │    '5.5±0.01µs'    │  '**5.4±0.01µs**'   │   '+2.0'   │
│    3    │            'Array access (Full)'             │ '**251.7±0.77µs**' │   '265.4±0.45µs'    │   '-4.8'   │
│    4    │         'Array creation (Execution)'         │  '**2.5±0.00ms**'  │    '2.6±0.00ms'     │   '-2.0'   │
│    5    │           'Array creation (Full)'            │  '**2.8±0.00ms**'  │    '2.8±0.00ms'     │   '-2.0'   │
│    6    │           'Array pop (Execution)'            │ '**815.0±2.66µs**' │   '833.0±3.43µs'    │   '-2.0'   │
│    7    │              'Array pop (Full)'              │  '1219.0±1.43µs'   │   '1216.7±1.65µs'   │   '0.0'    │
│    8    │     'Boolean Object Access (Execution)'      │  '**4.0±0.01µs**'  │    '4.1±0.01µs'     │   '-2.9'   │
│    9    │        'Boolean Object Access (Full)'        │ '**248.3±0.49µs**' │   '257.1±0.31µs'    │   '-3.8'   │
│   10    │            'Clean js (Execution)'            │   '642.9±3.95µs'   │ '**638.9±3.25µs**'  │   '+1.0'   │
│   11    │              'Clean js (Full)'               │ '**924.5±3.17µs**' │   '934.5±2.60µs'    │  '-0.99'   │
│   12    │             'Clean js (Parser)'              │   '31.3±0.03µs'    │    '31.3±0.04µs'    │   '0.0'    │
│   13    │                'Create Realm'                │   '368.5±0.34ns'   │ '**359.9±0.32ns**'  │   '+2.0'   │
│   14    │ 'Dynamic Object Property Access (Execution)' │    '4.2±0.01µs'    │    '4.2±0.02µs'     │   '0.0'    │
│   15    │   'Dynamic Object Property Access (Full)'    │ '**254.7±0.72µs**' │   '262.2±0.64µs'    │   '-2.9'   │
│   16    │            'Expression (Parser)'             │  '**5.2±0.01µs**'  │    '5.3±0.00µs'     │  '-0.99'   │
│   17    │           'Fibonacci (Execution)'            │ '**651.7±2.40µs**' │   '657.6±1.29µs'    │  '-0.99'   │
│   18    │              'Fibonacci (Full)'              │ '**933.2±1.15µs**' │   '938.9±1.00µs'    │  '-0.99'   │
│   19    │            'For loop (Execution)'            │ '**16.7±0.18µs**'  │    '16.9±0.05µs'    │  '-0.99'   │
│   20    │              'For loop (Full)'               │ '**259.5±0.51µs**' │   '271.0±1.42µs'    │   '-3.8'   │
│   21    │             'For loop (Parser)'              │   '15.0±0.05µs'    │    '15.0±0.24µs'    │   '0.0'    │
│   22    │           'Goal Symbols (Parser)'            │   '10.9±0.03µs'    │  '**10.8±0.02µs**'  │   '+1.0'   │
│   23    │            'Hello World (Parser)'            │    '3.1±0.02µs'    │    '3.1±0.01µs'     │   '0.0'    │
│   24    │             'Long file (Parser)'             │  '753.0±17.43ns'   │ '**741.4±15.18ns**' │   '+2.0'   │
│   25    │            'Mini js (Execution)'             │   '591.7±6.23µs'   │   '590.2±3.77µs'    │   '0.0'    │
│   26    │               'Mini js (Full)'               │ '**862.5±4.50µs**' │   '878.0±2.61µs'    │   '-2.0'   │
│   27    │              'Mini js (Parser)'              │   '27.4±0.03µs'    │    '27.4±0.02µs'    │   '0.0'    │
│   28    │      'Number Object Access (Execution)'      │  '**3.1±0.01µs**'  │    '3.2±0.00µs'     │   '-2.0'   │
│   29    │        'Number Object Access (Full)'         │ '**246.2±2.76µs**' │   '256.7±0.35µs'    │   '-3.8'   │
│   30    │        'Object Creation (Execution)'         │  '**3.7±0.01µs**'  │    '3.7±0.02µs'     │   '-2.0'   │
│   31    │           'Object Creation (Full)'           │ '**250.4±0.75µs**' │   '259.9±1.75µs'    │   '-3.8'   │
│   32    │             'RegExp (Execution)'             │   '11.2±0.02µs'    │    '11.2±0.05µs'    │   '0.0'    │
│   33    │               'RegExp (Full)'                │ '**255.3±0.44µs**' │   '265.2±1.35µs'    │   '-3.8'   │
│   34    │         'RegExp Literal (Execution)'         │   '11.2±0.03µs'    │    '11.2±0.06µs'    │   '0.0'    │
│   35    │           'RegExp Literal (Full)'            │ '**262.8±0.60µs**' │   '274.0±0.48µs'    │   '-3.8'   │
│   36    │    'RegExp Literal Creation (Execution)'     │    '8.0±0.04µs'    │    '8.0±0.03µs'     │   '0.0'    │
│   37    │       'RegExp Literal Creation (Full)'       │ '**253.8±0.39µs**' │   '265.2±0.75µs'    │   '-4.8'   │
│   38    │ 'Static Object Property Access (Execution)'  │    '4.0±0.01µs'    │  '**4.0±0.01µs**'   │   '+1.0'   │
│   39    │    'Static Object Property Access (Full)'    │ '**250.4±0.54µs**' │   '260.3±0.36µs'    │   '-3.8'   │
│   40    │      'String Object Access (Execution)'      │    '6.0±0.02µs'    │    '6.0±0.03µs'     │   '0.0'    │
│   41    │        'String Object Access (Full)'         │ '**252.7±0.38µs**' │   '264.4±0.42µs'    │   '-4.8'   │
│   42    │       'String comparison (Execution)'        │  '**5.5±0.02µs**'  │    '5.7±0.02µs'     │   '-2.0'   │
│   43    │          'String comparison (Full)'          │ '**248.4±0.29µs**' │   '260.0±0.60µs'    │   '-4.8'   │
│   44    │      'String concatenation (Execution)'      │  '**4.2±0.01µs**'  │    '4.4±0.02µs'     │   '-3.8'   │
│   45    │        'String concatenation (Full)'         │ '**242.8±0.37µs**' │   '257.1±0.33µs'    │   '-5.7'   │
│   46    │          'String copy (Execution)'           │    '3.3±0.01µs'    │  '**3.3±0.02µs**'   │   '+1.0'   │
│   47    │             'String copy (Full)'             │ '**243.0±0.59µs**' │   '253.3±0.32µs'    │   '-3.8'   │
│   48    │            'Symbols (Execution)'             │  '**2.6±0.00µs**'  │    '2.7±0.02µs'     │   '-2.9'   │
│   49    │               'Symbols (Full)'               │ '**228.6±0.69µs**' │   '240.0±0.57µs'    │   '-4.8'   │
│   50    │                      ''                      │     undefined      │      undefined      │   '+NaN'   │
└─────────┴──────────────────────────────────────────────┴────────────────────┴─────────────────────┴────────────┘

Razican · 2021-09-01T07:09:44Z

boa/src/object/property_map.rs

+
+/// Wrapper around indexmap::IndexMap for usage in PropertyMap
+#[derive(Debug, Finalize)]
+struct OrderedHashMap<K: Trace>(IndexMap<K, PropertyDescriptor, BuildHasherDefault<FxHasher>>);


What is the benefit of using a new struct, comparing to just a type definition? I guess it's related to Trace + Finalize implementation, right?

You cannot implement foreign traits for foreign types. That's why a newtype is necessary.

Razican · 2021-09-01T18:02:33Z

Did we use sorting somewhere else? Do we need to remove that code somewhere?

raskad · 2021-09-01T18:05:13Z

Did we use sorting somewhere else? Do we need to remove that code somewhere?

As far as I saw, we only sort the 'Index' property map, and that is not changed here. But good point, I will check to codebase for that.

raskad · 2021-09-01T21:14:37Z

Added a test based on the 262 Object.keys and Object.values order tests. @jedel1043

Found an unneeded sort that I implemented myself a few days back :D Thanks @Razican

jedel1043

Well done! The small improvements in performance were a nice bonus :)

Replace FxHashMap with IndexMap in object properties

00dcacd

raskad added bug Something isn't working execution Issues or PRs related to code execution labels Sep 1, 2021

raskad marked this pull request as ready for review September 1, 2021 02:10

raskad requested review from jedel1043, HalidOdat and Razican September 1, 2021 02:10

jedel1043 reviewed Sep 1, 2021

View reviewed changes

Razican approved these changes Sep 1, 2021

View reviewed changes

Razican added this to the v0.13.0 milestone Sep 1, 2021

raskad added 2 commits September 1, 2021 21:54

Add test for property order

684b3b4

Remove unneeded sort from JSON.stringify

abbec05

jedel1043 approved these changes Sep 1, 2021

View reviewed changes

Razican merged commit 6115182 into boa-dev:master Sep 1, 2021

raskad deleted the property-order-store branch September 10, 2021 23:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `FxHashMap` with `IndexMap` in object properties #1547

Replace `FxHashMap` with `IndexMap` in object properties #1547

raskad commented Sep 1, 2021 •

edited

Loading

raskad commented Sep 1, 2021 •

edited

Loading

jedel1043 left a comment •

edited

Loading

Razican commented Sep 1, 2021

Razican Sep 1, 2021

jedel1043 Sep 1, 2021

Razican commented Sep 1, 2021

raskad commented Sep 1, 2021

raskad commented Sep 1, 2021

jedel1043 left a comment

Replace FxHashMap with IndexMap in object properties #1547

Replace FxHashMap with IndexMap in object properties #1547

Conversation

raskad commented Sep 1, 2021 • edited Loading

raskad commented Sep 1, 2021 • edited Loading

Test262 conformance changes:

jedel1043 left a comment • edited Loading

Choose a reason for hiding this comment

Razican commented Sep 1, 2021

Benchmarks

Razican Sep 1, 2021

Choose a reason for hiding this comment

jedel1043 Sep 1, 2021

Choose a reason for hiding this comment

Razican commented Sep 1, 2021

raskad commented Sep 1, 2021

raskad commented Sep 1, 2021

jedel1043 left a comment

Choose a reason for hiding this comment

Replace `FxHashMap` with `IndexMap` in object properties #1547

Replace `FxHashMap` with `IndexMap` in object properties #1547

raskad commented Sep 1, 2021 •

edited

Loading

raskad commented Sep 1, 2021 •

edited

Loading

jedel1043 left a comment •

edited

Loading